refactor: Use ResizeObserver for responsive charts - #8017
Conversation
|
I also want to confirm this description; it seems this option still exists in v4: plotly.js/src/plot_api/plot_config.js Lines 127 to 134 in acddada |
|
There is one failed check:
I'm not sure whether this is related to my changes. If it is, how should I address it, or can it be ignored? |
ResizeObserver for responsive chartsResizeObserver for responsive charts
camdecoster
left a comment
There was a problem hiding this comment.
Thanks for this quick PR to make this update! There are a few changes that need to be made, but it's most of the way there. In addition to what you've done, could you also do the following?
- Update the
responsiveattribute description to be more general? Something like this:'Determines whether to change the layout size when the **graph container** is resized.' - Add some tests to cover the new behavior
There was a problem hiding this comment.
It turns out that we still need the window resize listener for the fillFrame config option. That's my mistake from my previous comments.
| if (!gd._responsiveChartObserver) { | ||
| // Keep a reference to the resize observer to purge it down the road | ||
| gd._responsiveChartObserver = new ResizeObserver(function() { | ||
| if (!Lib.isHidden(gd)) Plots.resize(gd); | ||
| }; | ||
| }); | ||
|
|
||
| // Listen to window resize | ||
| window.addEventListener('resize', gd._responsiveChartHandler); | ||
| gd._responsiveChartObserver.observe(gd); | ||
| } |
There was a problem hiding this comment.
It turns out that we still need the window resize listener for the fillFrame config option. That's my mistake from my previous comments. The ResizeObserver check probably isn't necessary, but we might as well since we already need that branch for fillFrame. Let me know what you think of this plan.
| if (!gd._responsiveChartObserver) { | |
| // Keep a reference to the resize observer to purge it down the road | |
| gd._responsiveChartObserver = new ResizeObserver(function() { | |
| if (!Lib.isHidden(gd)) Plots.resize(gd); | |
| }; | |
| }); | |
| // Listen to window resize | |
| window.addEventListener('resize', gd._responsiveChartHandler); | |
| gd._responsiveChartObserver.observe(gd); | |
| } | |
| if (!gd._clearResponsive) { | |
| const resizeIfShown = () => { | |
| if (!Lib.isHidden(gd)) Plots.resize(gd); | |
| }; | |
| // We still need the window resize listener for `fillFrame` and an | |
| // escape hatch for browser-like users that don't support `ResizeObserver` (like jsdom) | |
| if (gd._context.fillFrame || typeof ResizeObserver === 'undefined') { | |
| window.addEventListener('resize', resizeIfShown); | |
| gd._clearResponsive = () => window.removeEventListener('resize', resizeIfShown); | |
| } else { | |
| let previousWidth = gd.offsetWidth; | |
| let previousHeight = gd.offsetHeight; | |
| const observer = new ResizeObserver(() => { | |
| const width = gd.offsetWidth; | |
| const height = gd.offsetHeight; | |
| // Ignore size changes of one pixel or less (the same as plotAutoSize) | |
| const changed = Math.abs(width - previousWidth) > 1 || Math.abs(height - previousHeight) > 1; | |
| previousWidth = width; | |
| previousHeight = height; | |
| // Only resize plot if it changed and is visible (width and height > 0) | |
| if (changed && width && height) resizeIfShown(); | |
| }); | |
| observer.observe(gd); | |
| gd._clearResponsive = () => observer.disconnect(); | |
| } | |
| } |
| @@ -167,14 +167,13 @@ function _doPlot(gd, data, layout, config) { | |||
|
|
|||
| // make the figure responsive | |||
There was a problem hiding this comment.
| // make the figure responsive | |
| // Make the figure responsive. We need to save the callback that clears the | |
| // listener for proper teardown. |
| if(gd._responsiveChartObserver) { | ||
| gd._responsiveChartObserver.disconnect(); | ||
| delete gd._responsiveChartObserver; | ||
| } |
There was a problem hiding this comment.
With the changes I suggested in plot_api.js, this can be simplified to the following:
| if(gd._responsiveChartObserver) { | |
| gd._responsiveChartObserver.disconnect(); | |
| delete gd._responsiveChartObserver; | |
| } | |
| if (gd._clearResponsive) { | |
| gd._clearResponsive(); | |
| delete gd._clearResponsive; | |
| } |
Original discussion: plotly/react-plotly.js#380
Related PR: plotly/react-plotly.js#379
This is a follow-up to #2974.
Closes #7059.
Based on plotly/react-plotly.js#380 (comment), I'm opening this PR in plotly.js as well so
config.responsivecan use ResizeObserver directly.